Why are the labels reversed before sorting?

They only get reversed in the case of bfill - it's the cheapest way to fill backwards that I could think of

Right, but doesn't the .sort() undo the reversing?

I took that from the below SO article and the comment by perimosocordiae was helpful in figuring out what's going on. Open to changing it to sorted_labels = np.sort(sorted_labels)[::-1] for readability if you'd like

https://stackoverflow.com/questions/26984414/efficiently-sorting-a-numpy-array-in-descending-order

Oh, thanks! Perhaps too clever, how about this? still avoids the copy

sorted_labels.sort()
sorted_labels = sorted_labels[::-1]

Actually I think you are on to something - the extra sort doesn't feel right and I think it could cause a bug as it will really just return an array ranging from N-1..0. Going to work up some test cases to validate and add fix to next commit

Won't filled_values need to be tracked by group? Consider this case (haven't tested on your PR):

df = pd.DataFrame({
'k': [1, 1, 2, 2, 1, 1], 'v': [1.1, np.nan, 1, np.nan, np.nan, np.nan]})

df.groupby('k').ffill(limit=2)
Out[25]: 
   k    v
0  1  1.1
1  1  1.1
2  2  1.0
3  2  1.0
4  1  1.1
5  1  NaN

It needs to be tracked by both group and value. One thing to keep in mind is that the main loop in the Cython function doesn't go sequentially over the values in the series / frame, but rather iterates over the argsorteded labels, which allows you to keep track of groups easily. Just ran your example on my PR and it gave the result you have above.

That said, I don't have a problem adding a test case that mixes the groups up to prevent this from breaking in the future. Can bundle that in with the next commit

The test case for this is now a little more complicated, handling both sequential groups (ex: ['a', 'a', 'a', 'b', 'b', 'b']) and "interwoven" (ex: ['a', 'b', 'a', 'b', 'a', 'b']). Open to splitting it up into separate tests if you think that would make it more readable.

Otherwise this instance should be covered. I fixed a bug in the Cython code to go along with it - thanks for the callout!

Rather than calling apply, pattern so far has been to define the method directly Groupby.cumsum, etc

Tied to the comment below, I added _cython_apply as its own function to handle dispatching back to the Cython layer, which this apply tries first before falling back to the current method of iterating over the groups and applying the method directly from there. Items like Groupby.cumsum call _cython_transform in a similar fashion, which I was trying to emulate here but I was hesitant to send these methods down the transform route because of the complexity that would cause there

I think all this should pass down to _cython_transform?

I was a little torn over this. The call signature doesn't play all that nicely with the other methods utilizing _cython_transform. Specifically, the is_numeric and is_datetimelike arguments aren't applicable and there are some conditionals that need to be added to prevent accidental casting of data during the fill. On top of that, we'd have to wrap the transformation to be sure to include the grouped columns as none of the other transformations do that.

Touched on this with @jreback in the comments of #19481 - there for sure needs to be a pretty comprehensive refactoring on groupby.py to more effectively dispatch Cython operations. For the time being I thought this approach would be the "best of the worst" option, but can take another look at wiring into transform if we feel that is a show stopper

I don't disagree the whole thing needs some refactoring, xref also to #19354 - I'll think about it a bit more too

copy as in #19701

What's this if branch for?

Fill is unique in that unlike say transformations the column(s) used in the grouping are also returned as part of the output of a DataFrame object. _iterate_slices does not go over those items and I didn't see an existing method to handle that, so this conditional makes sure the grouped columns still make their way into the frame that gets returned

FWIW I was thinking to myself if its worth adding an instance method to the GroupBy object to handle this (_iterate_groups or something to the effect) but couldn't think of any other function that requires this feature

rather than doing the if type(...), make a method in SeriesGroupBy and DataFrameGroupBy

do we bench for the compat methods for datetimelikes? (timestamp / timedelta), most of these would work (except prod, mean, mad, pct_change and a few more), though these likely work for timedeltas. ok with adding an issue to do this as well (IOW doesn't need to be in this PR).

See #19733

I am not sure this will render. you can just do .groupby().ffill() and so on

curr_fill_idx seems potentially not defined (if it is missing but the limit is not hit), not sure if that is possible. maybe could initialize curr_fill_idx = -1?

The initialization occurs at declaration. Can move that into the body of the function if you feel that is more readable

ahh see that ok then

maybe want an assert that N == len(labels) == len(mask)

rather than doing the if type(...), make a method in SeriesGroupBy and DataFrameGroupBy

maybe make a routine that shares code with how the shift_indexer is called (I am talking about in python space here)

ahh see that ok then

blank line here

these should be passed in directly (from the calling function), no? rather than hard coded inside this function.

Right, maybe the caller passes in a partial the kwargs baked rather than the string version of the funtion?

_get_cythonized_result(
    partial(pd._libs.groupby.group_shift_indexer, nperiods=2),
    ...
)

I’ll think more about that. The one complicating factor is that masked values cannot be passed in directly as they are calculated on a per slice basis. To avoid a convoluted set of calls I figured it would make sense to have all keywords and positional arguments resolved within one function, but there’s certainly other ways to go about this

@chris-b1 comment is about the exp_kwds, I am still not clear why the caller cannot pass these

you can pass kwargs to cython functions, so not sure this is necessary

I explicitly converted kwargs into positional arguments from the first comment back in #19481. If we are OK with kwargs in the Cython layer I can rewrite this, as it could simplify a few things

yes, you can pass kwargs

do we need to specify int64? (or platform int)?

yes, int64

This was a copy/paste of existing code, so I didn't give it too much thought. That said, labels is already wrapped with a _ensure_int64 call as part of group_info within the BaseGrouper, so unless there's something in particular you know of I figure it would be easiest to inherit that type from labels and not override

would be slightly simpler as a list-comprehension

though maybe this should be an arg to _fill? (IOW the option to do it should be done in fill)

Do you mean as an arg to _get_cythonized_result (fill would always use, unless we wanted to implement a new feature)? I was thinking about that but given fill on a DataFrameGroupBy is the only operation I know of at the moment that includes the grouping in the body of the returned object I figured it made the most sense to just perform that action there

yes, int64

Right, maybe the caller passes in a partial the kwargs baked rather than the string version of the funtion?

_get_cythonized_result(
    partial(pd._libs.groupby.group_shift_indexer, nperiods=2),
    ...
)

Contrary to the int64 case, you do want to use view here - numpy doesn't see bool and uint8 as the 'same' type, so will always copy, which view elides.

Thanks for the heads up - still trying to wrap my head around some of the nuances there. Out of curiosity, how do you know this? Just from experience or is there a good reference?

I do see that astype documentation mentions dtype, order and subok requirements need to be satisfied to prevent copy, but I'm not clear on what those requirements are and if they are documented

Unfortunately all I can really point to is trial and error. Some "general" rules, at least in a pandas context

1. copy=False will only elide a copy if the dtype is identical (may be contradictions to this, but at least not in general). Useful for conditional casts.
2. arr.view(dtype) never copies. Viewing across types should only be used if if the itemsize of the two types are identical. Primarily useful for casting a type with some metadata down to a a more primitive type (bool to uint8, datetime64 to int64).

can you add a doc-string

@chris-b1 comment is about the exp_kwds, I am still not clear why the caller cannot pass these

yes, you can pass kwargs

hmm, if you are not doing a .get() here, is there a reason you are not simply haveing periods=None in the signature? (if its optional)?

same again why not simply list them in the signature? (you can have **kwargs if you want to ignore other ones)

I didn't want to define a default value for something like periods in both the Python and Cython layers, leaving the former to set that default and be responsible for passing to Cython.

It's a pretty trivial change so will re-push with your changes reflected soon

Think I've been looking at this change too long...planning on making these required in the Cython signature and using the existing defaults in the Python layer to populate those via **kwargs. If we are not on the same page let me know

no that sounds good